chore: enable unused-vars and import lint rules#4083
Closed
carderne wants to merge 1 commit into
Closed
Conversation
|
Contributor
|
Important Review skippedToo many files! This PR contains 965 files, which is 815 over the limit of 150. To get a review, narrow the scope: Upgrade to a paid plan to raise the limit. ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (965)
You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
@trigger.dev/build
trigger.dev
@trigger.dev/core
@trigger.dev/python
@trigger.dev/react-hooks
@trigger.dev/redis-worker
@trigger.dev/rsc
@trigger.dev/schema-to-json
@trigger.dev/sdk
commit: |
carderne
force-pushed
the
chore/more-linting
branch
from
74a3840 to
683a6c5
Compare
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
carderne
force-pushed
the
chore/more-linting
branch
from
683a6c5 to
b344176
Compare
Contributor
There was a problem hiding this comment.
🚩 SSO page removes background polling — may leave stale state during setup
The SSO settings route (apps/webapp/app/routes/_app.orgs.$organizationSlug.settings.sso/route.tsx) removed the useEffect polling interval that refreshed domain/connection state every 5 seconds while setup was incomplete. Previously, when a user was finishing steps in the admin portal (another tab), the page would auto-update. Now the page stays stale until a manual reload. The shouldPoll logic, revalidator, and setPortalIntent state were all removed. This is a UX regression for the SSO setup flow — users completing domain verification in another tab won't see the page reflect their progress.
Was this helpful? React with 👍 or 👎 to provide feedback.
Comment on lines
+189
to
+190
| submission.error.key = ["Project not found"]; | ||
| return json(submission); |
Contributor
There was a problem hiding this comment.
🟡 Server-side form errors silently lost because they are stored under a non-existent field name instead of the form-level error key
Server error messages are stored under the wrong object key (submission.error.key at alerts.new/route.tsx:189) instead of the form-level key submission.error[""], so the <FormError>{form.error}</FormError> component never displays them.
Impact: Users performing actions that hit error conditions (e.g. project not found, alert channel creation failure) see no error feedback — the form silently fails.
Conform v0.9 form-level error mechanism
In conform v0.9, form.error on the client reads from submission.error[""] (the empty-string key). Setting submission.error.key = ["..."] stores the error under a field name called "key", but the alerts form has no field named "key", so no <FormError id={...}> renders it. The correct migration of conform v1's submission.reply({ formErrors: [...] }) is submission.error[""] = [...], which IS used correctly in resources.$projectId.deployments.$deploymentShortCode.promote.ts:96.
The same bug exists at:
alerts.new/route.tsx:201("Failed to create alert channel")alerts/route.tsx:128("Project not found")environment-variables.new/route.tsx:166("Project not found")
Suggested change
| submission.error.key = ["Project not found"]; | |
| return json(submission); | |
| submission.error["" ] = ["Project not found"]; | |
| return json(submission); | |
Was this helpful? React with 👍 or 👎 to provide feedback.
Comment on lines
+48
to
+49
| "@conform-to/react": "0.9.2", | ||
| "@conform-to/zod": "0.9.2", |
Contributor
There was a problem hiding this comment.
🚩 Conform v1→v0.9 downgrade is unusual and carries regression risk
The package.json change (apps/webapp/package.json:48-49) downgrades @conform-to/react and @conform-to/zod from ^1.2.2 to 0.9.2. Downgrading a form library by a major version is uncommon and carries risk: conform v1 had breaking API changes from v0.9 (different error shapes, different submission handling, different list manipulation APIs). The migration appears largely correct across ~40 files, but the sheer number of touch points (form.props vs getFormProps, field.error vs field.errors, lastSubmission vs lastResult, useFieldList vs getFieldList, etc.) makes it easy for subtle regressions to slip through. The error shape inconsistency (some routes use submission.error[""] correctly, others use submission.error.key) suggests the migration was partially automated without full manual review of each error path.
Was this helpful? React with 👍 or 👎 to provide feedback.
| @@ -99,8 +60,8 @@ export class Exec { | |||
|
|
|||
Contributor
There was a problem hiding this comment.
🚩 Exec credential redaction feature reverted without explanation
The redactArgsForLogging function in packages/core/src/v3/apps/exec.ts (and its test file exec.test.ts) was removed, and the associated changeset .changeset/tidy-exec-arg-logging.md was deleted. This reverts a security-hardening feature that masked --password, --token, and similar flag values from debug logs. The exec command now logs raw arguments including credentials at the debug level. While debug logs may not be user-facing in production, this is a deliberate reduction in defense-in-depth. No explanation is provided in the PR for the revert.
(Refers to lines 47-60)
Was this helpful? React with 👍 or 👎 to provide feedback.
Comment on lines
197
to
199
| concurrencyLimitPendingObservableGauge.addCallback( | ||
| this.#updateConcurrencyLimitPendingMetric.bind(this) | ||
| ); |
Contributor
There was a problem hiding this comment.
🚩 Oldest-message-age metric and Lua script reverted from redis-worker
The oldestMessageAge() method, its getOldestDueScore Lua script, the observable gauge redis_worker.queue.oldest_message_age, and the associated test were all removed from packages/redis-worker. The changeset .changeset/redis-worker-oldest-message-age.md was also deleted. This was a queue-stall diagnostic metric. If any dashboards or alerts in production reference this metric, they will silently stop receiving data after this change ships.
Was this helpful? React with 👍 or 👎 to provide feedback.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Enable oxlint rules:
no-unused-vars,consistent-type-imports,import/no-duplicatesEnables three previously-disabled oxlint rules and fixes all violations:
no-unused-vars— unused imports removed, unused params/vars prefixed_;caughtErrorsIgnorePattern/destructuredArrayIgnorePatternadded to config so_-prefixed catch params are properly ignoredtypescript/consistent-type-imports— enforced by oxlint--fix; inlineimport()annotations suppressed with targetedeslint-disablecommentsimport/no-duplicates— duplicate imports merged per moduleAll changes are no-ops: no behaviour changes, typecheck passes.